Conversation
- Improved `get_agent_ledger` method in `ledger.py` by adding validation for `agent_id` to raise a `ValueError` if it is empty. - Updated `_get_output_path` in `monitor.py` to include detailed path sanitization steps and added checks to prevent path traversal attacks. - Refined agent name sanitation and fallback techniques to ensure unique naming. - Incremented version number to `1.5.114` in `pyproject.toml` and `uv.lock`.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughBump praisonai/praisonaiagents versions from 4.5.113/1.5.113 to 4.5.114/1.5.114 across Dockerfiles, deployment files, and package configurations. Add input validation and security hardening for agent IDs, file paths, code execution sandbox, YAML parsing, filesystem operations, and API authentication. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoSecurity hardening with input validation, path traversal prevention, and authentication
WalkthroughsDescription• Enhanced security with comprehensive input validation and sanitization - Added agent_id validation in ledger to reject empty strings - Implemented path traversal prevention in monitor output paths - Added filepath and command sanitization in agent tools - Secured A2U server endpoints with bearer token authentication • Expanded dangerous attribute/function blocking in Python sandbox - Added 18+ restricted attributes for frame/code introspection - Extended blocked function calls (input, breakpoint, setattr, delattr, dir) • Improved YAML parser security with property whitelisting - Added allowed keys validation to prevent injection attacks - Enforced file size limits and path traversal checks • Version bump to 4.5.114 across all packages and configurations Diagramflowchart LR
Input["User Input<br/>Filepath/Command/YAML"] -->|Sanitize| Validate["Validation Layer<br/>Path Traversal<br/>Injection Detection"]
Validate -->|Safe| Process["Process Request<br/>Execute/Read/Write"]
Validate -->|Unsafe| Reject["Reject with<br/>ValueError"]
Auth["A2U Requests"] -->|Check Token| AuthCheck["Bearer Token<br/>Authentication"]
AuthCheck -->|Valid| AllowReq["Allow Request"]
AuthCheck -->|Invalid| DenyReq["401/403 Response"]
Sandbox["Python Code"] -->|Block| BlockedAttrs["Restricted Attributes<br/>Frame/Code Objects"]
BlockedAttrs -->|Prevent| Escape["Prevent Sandbox<br/>Escape"]
File Changes1. src/praisonai-agents/praisonaiagents/context/ledger.py
|
Code Review by Qodo
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the project version to 4.5.114 and introduces significant security enhancements, including path traversal prevention, command sanitization, and Bearer token authentication for A2U server routes. The Python execution environment was also hardened by expanding the list of blocked attributes and functions. Review feedback identified several critical security and performance improvements: the need to block 'getattr' to prevent dynamic attribute access, addressing symlink bypasses and CWD mismatches in TypeScript path validation, expanding the command sanitization blacklist to cover more injection vectors, and optimizing loop-level constant initialization.
| if node.func.id in ('exec', 'eval', 'compile', '__import__', | ||
| 'open', 'input', 'breakpoint', | ||
| 'setattr', 'delattr', 'dir'): |
There was a problem hiding this comment.
The getattr function is missing from the blocked functions list. Since setattr and delattr are blocked to prevent object manipulation, getattr should also be blocked to prevent unauthorized access to sensitive attributes (like __globals__ or __subclasses__) that might bypass the blocked_attrs check if accessed dynamically.
| if node.func.id in ('exec', 'eval', 'compile', '__import__', | |
| 'open', 'input', 'breakpoint', | |
| 'setattr', 'delattr', 'dir'): | |
| if node.func.id in ('exec', 'eval', 'compile', '__import__', | |
| 'open', 'input', 'breakpoint', 'getattr', | |
| 'setattr', 'delattr', 'dir'): |
| const resolvedFile = path.resolve(options.basePath, normalizedPath); | ||
| if (!resolvedFile.startsWith(resolvedBase + path.sep) && resolvedFile !== resolvedBase) { | ||
| throw new Error(`File path must be within base directory: ${options.basePath}`); | ||
| } | ||
| } | ||
|
|
||
| // SECURITY: Enforce file size limit (default 1 MB) | ||
| const maxSize = options.maxFileSizeBytes ?? 1_048_576; | ||
| const stat = await fs.stat(filePath); | ||
| if (stat.size > maxSize) { | ||
| throw new Error(`File too large: ${stat.size} bytes exceeds limit of ${maxSize} bytes`); | ||
| } | ||
|
|
||
| const content = await fs.readFile(filePath, 'utf-8'); |
There was a problem hiding this comment.
There are two issues here:
- Symlink Bypass:
path.resolvedoes not resolve symbolic links. An attacker could provide a path to a symlink that points outside thebasePath, bypassing the prefix check. Usefs.realpath()to resolve the actual path on disk before validation. - CWD Mismatch: If
basePathis provided, the code validatesresolvedFilebut then reads fromfilePathat line 305. IffilePathis a relative path,fs.readFilewill resolve it against the process's current working directory (CWD) rather than the intendedbasePath.
| DANGEROUS_PATTERNS = [ | ||
| '$(', '`', # Command substitution | ||
| '&&', '||', # Command chaining | ||
| '>>', '> ', # Output redirection | ||
| '| ', # Pipe | ||
| ] |
There was a problem hiding this comment.
The DANGEROUS_PATTERNS blacklist is incomplete and easily bypassed. For example, it checks for '| ' (with a space) but not '|' (without a space), allowing command piping like ls|grep. It also misses critical shell metacharacters like ;, &, and newlines which are checked in _sanitize_filepath but omitted here.
DANGEROUS_PATTERNS = [
'$(', '`', # Command substitution
'&&', '||', # Command chaining
'>>', '>', # Output redirection
'|', ';', '&', # Pipe and separators
'\n', '\r' # Line breaks
]| # Verify the resolved path stays within the expected parent dir | ||
| resolved = result.resolve() | ||
| parent_resolved = self.path.parent.resolve() | ||
| if not str(resolved).startswith(str(parent_resolved) + '/') and resolved.parent != parent_resolved: |
There was a problem hiding this comment.
The path validation logic uses string concatenation with a forward slash (/), which is not cross-platform compatible and will fail on Windows where the path separator is \. Since the project requires Python 3.10+, you should use the idiomatic is_relative_to method for a robust, platform-independent check.
| if not str(resolved).startswith(str(parent_resolved) + '/') and resolved.parent != parent_resolved: | |
| if not resolved.is_relative_to(parent_resolved): |
| const ALLOWED_STEP_KEYS = new Set([ | ||
| 'type', 'agent', 'tool', 'input', 'output', 'condition', | ||
| 'onError', 'maxRetries', 'timeout', 'loopCondition', 'maxIterations', | ||
| ]); |
There was a problem hiding this comment.
The ALLOWED_STEP_KEYS Set is being re-initialized on every iteration of the line-parsing loop. This is inefficient. It should be defined outside the loop or as a static constant to improve performance.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // If basePath is specified, ensure resolvedPath stays within it | ||
| if (options.basePath) { | ||
| const resolvedBase = path.resolve(options.basePath); | ||
| const resolvedFile = path.resolve(options.basePath, normalizedPath); | ||
| if (!resolvedFile.startsWith(resolvedBase + path.sep) && resolvedFile !== resolvedBase) { | ||
| throw new Error(`File path must be within base directory: ${options.basePath}`); | ||
| } | ||
| } | ||
|
|
||
| // SECURITY: Enforce file size limit (default 1 MB) | ||
| const maxSize = options.maxFileSizeBytes ?? 1_048_576; | ||
| const stat = await fs.stat(filePath); | ||
| if (stat.size > maxSize) { | ||
| throw new Error(`File too large: ${stat.size} bytes exceeds limit of ${maxSize} bytes`); | ||
| } | ||
|
|
||
| const content = await fs.readFile(filePath, 'utf-8'); | ||
| const definition = parseYAMLWorkflow(content); | ||
| return createWorkflowFromYAML(definition, agents, tools); |
There was a problem hiding this comment.
1. Basepath check not enforced 🐞 Bug ⛨ Security
In loadWorkflowFromFile(), the basePath guard validates a resolved path (resolvedFile) but then size-checks and reads using the original filePath, so the code may read a different file than the one validated (depending on process CWD). This undermines the intended basePath restriction and can load unintended workflow files.
Agent Prompt
### Issue description
`loadWorkflowFromFile()` validates `resolvedFile` (based on `options.basePath` + `normalizedPath`) but then uses `filePath` for `fs.stat()` and `fs.readFile()`. This means the security check may not apply to the actual file being accessed.
### Issue Context
When `options.basePath` is provided, the function should consistently treat the file to load as `resolvedFile` (and ideally also return/operate on that canonical path).
### Fix Focus Areas
- src/praisonai-ts/src/workflows/yaml-parser.ts[283-307]
### Implementation notes
- Compute a single `const effectivePath = options.basePath ? resolvedFile : normalizedOrAbsolutePath`.
- Use `effectivePath` for `fs.stat()` and `fs.readFile()`.
- Consider also returning/using `effectivePath` for any subsequent logic so logs/errors refer to the canonical path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above first. Review areas:
|
There was a problem hiding this comment.
Pull request overview
This PR bumps PraisonAI / PraisonAIAgents versions and adds several security hardening measures across the Python server/tools, the agents package, and the TypeScript YAML workflow loader.
Changes:
- Bump
praisonaito4.5.114andpraisonaiagentsto1.5.114across package metadata, lockfiles, and Docker/Homebrew artifacts. - Add optional bearer-token auth for A2U SSE endpoints and add sanitization/containment checks for CLI interactive file/command tooling.
- Harden workflow YAML parsing/file loading and agent sandbox/path handling (TS workflow loader, agent context snapshot filenames, Python sandbox AST restrictions, ledger input validation).
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/praisonai/uv.lock | Bumps locked praisonaiagents version to 1.5.114. |
| src/praisonai/pyproject.toml | Raises dependency floor for praisonaiagents to >=1.5.114. |
| src/praisonai/praisonai/version.py | Bumps praisonai version to 4.5.114. |
| src/praisonai/praisonai/endpoints/a2u_server.py | Adds optional bearer-token authentication gate for A2U routes. |
| src/praisonai/praisonai/deploy.py | Updates Dockerfile generator to install praisonai==4.5.114. |
| src/praisonai/praisonai/cli/features/agent_tools.py | Adds path/command sanitization and workspace containment checks for interactive tools. |
| src/praisonai/praisonai.rb | Updates Homebrew formula URL/tag to v4.5.114. |
| src/praisonai-ts/src/workflows/yaml-parser.ts | Adds key whitelisting and file path/file size safety checks for workflow loading. |
| src/praisonai-agents/uv.lock | Bumps locked praisonaiagents version to 1.5.114. |
| src/praisonai-agents/pyproject.toml | Bumps praisonaiagents package version to 1.5.114. |
| src/praisonai-agents/praisonaiagents/tools/python_tools.py | Expands AST sandbox restrictions (blocked attrs + blocked builtins). |
| src/praisonai-agents/praisonaiagents/context/monitor.py | Improves agent-name sanitization for snapshot output paths. |
| src/praisonai-agents/praisonaiagents/context/ledger.py | Adds validation for empty/whitespace agent IDs. |
| docker/Dockerfile.ui | Updates praisonai>=4.5.114. |
| docker/Dockerfile.dev | Updates praisonai>=4.5.114. |
| docker/Dockerfile.chat | Updates praisonai>=4.5.114. |
Comments suppressed due to low confidence (1)
src/praisonai/praisonai/cli/features/agent_tools.py:564
list_files()computesresolved = path.resolve()for the security check, but then iterates withfor f in path.glob(pattern)and builds relative paths from those results. This can reintroduce non-normalized..segments and makes the listing inconsistent with the validated directory. Useresolved.glob(...)(or iterateresolved.rglob(...)) and compute relatives fromresolvedto return normalized paths.
files = []
for f in path.glob(pattern):
files.append({
"name": f.name,
"path": str(f.relative_to(runtime.config.workspace)),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from .action_orchestrator import ActionOrchestrator | ||
|
|
||
| import os | ||
| import re |
There was a problem hiding this comment.
re is imported but not used in this module, which will fail lint/type-check in most setups. Remove the unused import or use it where intended.
| import re |
| def _sanitize_filepath(filepath: str, workspace: str = None) -> str: | ||
| """Validate and sanitize a filepath against injection attacks. |
There was a problem hiding this comment.
_sanitize_filepath annotates workspace as str but defaults it to None. This is an incorrect type hint; update it to Optional[str] (and import Optional) so static type checkers don’t report a mismatch.
| # SECURITY: Ensure resolved path stays within workspace | ||
| resolved = path.resolve() | ||
| ws_resolved = Path(runtime.config.workspace).resolve() | ||
| if not str(resolved).startswith(str(ws_resolved) + os.sep) and resolved != ws_resolved: |
There was a problem hiding this comment.
The workspace escape check uses str(resolved).startswith(str(ws_resolved) + os.sep), which is error-prone across platforms (case-insensitive FS, separator differences) and can be brittle. Prefer resolved.relative_to(ws_resolved) / Path.is_relative_to() (or os.path.commonpath) to enforce containment robustly.
| if not str(resolved).startswith(str(ws_resolved) + os.sep) and resolved != ws_resolved: | |
| try: | |
| resolved.relative_to(ws_resolved) | |
| except ValueError: |
| // Step properties — whitelist allowed keys to prevent injection | ||
| const ALLOWED_STEP_KEYS = new Set([ | ||
| 'type', 'agent', 'tool', 'input', 'output', 'condition', | ||
| 'onError', 'maxRetries', 'timeout', 'loopCondition', 'maxIterations', | ||
| ]); |
There was a problem hiding this comment.
ALLOWED_STEP_KEYS is re-created on every parsed step property line, which is unnecessary work and makes the loop harder to read. Move the Set constant outside the loop (module-level or function-level) so it’s created once.
| // SECURITY: Prevent path traversal | ||
| const normalizedPath = path.normalize(filePath); | ||
| if (normalizedPath.includes('..')) { | ||
| throw new Error('Path traversal detected: ".." is not allowed in file paths'); | ||
| } |
There was a problem hiding this comment.
The path traversal check normalizedPath.includes('..') can false-positive on filenames like foo..bar.yml and doesn’t ensure .. is a path segment. Instead, split normalizedPath by path.sep (and path.posix.sep if needed) and reject only segments equal to '..'.
| // If basePath is specified, ensure resolvedPath stays within it | ||
| if (options.basePath) { | ||
| const resolvedBase = path.resolve(options.basePath); | ||
| const resolvedFile = path.resolve(options.basePath, normalizedPath); | ||
| if (!resolvedFile.startsWith(resolvedBase + path.sep) && resolvedFile !== resolvedBase) { | ||
| throw new Error(`File path must be within base directory: ${options.basePath}`); | ||
| } | ||
| } | ||
|
|
||
| // SECURITY: Enforce file size limit (default 1 MB) | ||
| const maxSize = options.maxFileSizeBytes ?? 1_048_576; | ||
| const stat = await fs.stat(filePath); | ||
| if (stat.size > maxSize) { | ||
| throw new Error(`File too large: ${stat.size} bytes exceeds limit of ${maxSize} bytes`); | ||
| } | ||
|
|
||
| const content = await fs.readFile(filePath, 'utf-8'); | ||
| const definition = parseYAMLWorkflow(content); |
There was a problem hiding this comment.
When options.basePath is provided, you validate resolvedFile but then call fs.stat(filePath) and fs.readFile(filePath). This means the code can read a different file than the one you validated (depending on CWD / relative paths). Use the same resolved/validated path for stat + read (and consider resolving even when basePath is not provided for consistency).
| // If basePath is specified, ensure resolvedPath stays within it | ||
| if (options.basePath) { | ||
| const resolvedBase = path.resolve(options.basePath); | ||
| const resolvedFile = path.resolve(options.basePath, normalizedPath); | ||
| if (!resolvedFile.startsWith(resolvedBase + path.sep) && resolvedFile !== resolvedBase) { | ||
| throw new Error(`File path must be within base directory: ${options.basePath}`); | ||
| } | ||
| } | ||
|
|
||
| // SECURITY: Enforce file size limit (default 1 MB) | ||
| const maxSize = options.maxFileSizeBytes ?? 1_048_576; | ||
| const stat = await fs.stat(filePath); | ||
| if (stat.size > maxSize) { | ||
| throw new Error(`File too large: ${stat.size} bytes exceeds limit of ${maxSize} bytes`); | ||
| } | ||
|
|
||
| const content = await fs.readFile(filePath, 'utf-8'); |
There was a problem hiding this comment.
The new options behavior (basePath restriction + file size limit) in loadWorkflowFromFile() isn’t covered by existing unit tests (there are tests for parseYAMLWorkflow, but none for file-loading). Add tests that assert traversal is rejected, basePath containment is enforced, and oversize files are rejected.
| // If basePath is specified, ensure resolvedPath stays within it | |
| if (options.basePath) { | |
| const resolvedBase = path.resolve(options.basePath); | |
| const resolvedFile = path.resolve(options.basePath, normalizedPath); | |
| if (!resolvedFile.startsWith(resolvedBase + path.sep) && resolvedFile !== resolvedBase) { | |
| throw new Error(`File path must be within base directory: ${options.basePath}`); | |
| } | |
| } | |
| // SECURITY: Enforce file size limit (default 1 MB) | |
| const maxSize = options.maxFileSizeBytes ?? 1_048_576; | |
| const stat = await fs.stat(filePath); | |
| if (stat.size > maxSize) { | |
| throw new Error(`File too large: ${stat.size} bytes exceeds limit of ${maxSize} bytes`); | |
| } | |
| const content = await fs.readFile(filePath, 'utf-8'); | |
| let resolvedPath: string; | |
| // If basePath is specified, ensure resolvedPath stays within it | |
| if (options.basePath) { | |
| const resolvedBase = path.resolve(options.basePath); | |
| resolvedPath = path.resolve(resolvedBase, normalizedPath); | |
| if (!resolvedPath.startsWith(resolvedBase + path.sep) && resolvedPath !== resolvedBase) { | |
| throw new Error(`File path must be within base directory: ${options.basePath}`); | |
| } | |
| } else { | |
| resolvedPath = path.resolve(normalizedPath); | |
| } | |
| // SECURITY: Enforce file size limit (default 1 MB) | |
| const maxSize = options.maxFileSizeBytes ?? 1_048_576; | |
| const stat = await fs.stat(resolvedPath); | |
| if (stat.size > maxSize) { | |
| throw new Error(`File too large: ${stat.size} bytes exceeds limit of ${maxSize} bytes`); | |
| } | |
| const content = await fs.readFile(resolvedPath, 'utf-8'); |
| # Verify the resolved path stays within the expected parent dir | ||
| resolved = result.resolve() | ||
| parent_resolved = self.path.parent.resolve() | ||
| if not str(resolved).startswith(str(parent_resolved) + '/') and resolved.parent != parent_resolved: |
There was a problem hiding this comment.
The directory containment check uses string prefix matching with a hard-coded '/' separator (startswith(str(parent_resolved) + '/')), which is not portable and can be brittle. Use resolved.is_relative_to(parent_resolved) / resolved.relative_to(parent_resolved) (Python >=3.9) or os.path.commonpath to verify the resolved path stays within the expected parent.
| if not str(resolved).startswith(str(parent_resolved) + '/') and resolved.parent != parent_resolved: | |
| try: | |
| resolved.relative_to(parent_resolved) | |
| except ValueError: |
|
@claude You are the FINAL architecture reviewer. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job • Final Architecture Review & Security Fixes ✅I've conducted the final architecture review and implemented critical security fixes based on reviewer feedback. Todo List:
Phase 1: Architecture Validation ✅Per AGENTS.md guidelines, this PR passes all checks:
Phase 2: Security Fixes Implemented ✅All critical issues from reviewers have been fixed: 1. Path Traversal Vulnerabilities (TypeScript) - FIXED
2. Python Sandbox Escape - FIXED
3. Command Injection Gaps - FIXED
4. Path Validation Improvements - FIXED
5. Code Quality Issues - FIXED
Phase 3: Final Verdict ✅ALL ISSUES RESOLVED - APPROVE PR ✅ Security: All critical vulnerabilities patched Changes committed to branch: The original PR merged successfully with version bump 4.5.114 and comprehensive security hardening. The additional fixes I've implemented address all valid reviewer concerns without introducing any regressions. Recommendation: ✅ APPROVED - This PR significantly improves PraisonAI's security posture while maintaining full backward compatibility. | Branch |
PR #1284: Changes from Claude
Summary by CodeRabbit
Release Notes
New Features
Security Improvements
Chores